HDDS-15342. Make RocksDB bottommost level compaction options configurable for background compaction#10419
Conversation
sarvekshayr
left a comment
There was a problem hiding this comment.
Thanks @ptlrs for working on this.
…configuration logic.
…nfiguration and update its default type.
|
Failed acceptance (tools) CI - |
ashishkumar50
left a comment
There was a problem hiding this comment.
@ptlrs Thanks for the update, LGTM.
|
@ptlrs Can you please look at test failure, looks related. |
|
@ashishkumar50 as @sarvekshayr had also observed, this PR depends on #10428 which fixes the failing Robot test. Can we please merge that first? I may need to pull the changes in this PR after that. |
…ocksDB-BottommostLevelCompaction-options-configurable # Conflicts: # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactDBUtil.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompactionService.java
… in OzoneManager.
|
Hi @ashishkumar50 @sarvekshayr, could you please take another look? I have merged the changes from master. The robot test should now pass. |
| </description> | ||
| </property> | ||
| <property> | ||
| <name>ozone.om.compaction.service.bottommostlevelcompaction</name> |
There was a problem hiding this comment.
Please use - as separator in last part of the property name.
There was a problem hiding this comment.
Is this necessary? bottommostlevelcompaction is the original name as per org.rocksdb.CompactRangeOptions.BottommostLevelCompaction.
Most importantly, searching for bottommostlevelcompaction brings up all instances of it in both code and configuration in my IDE. Adding a separator would break that.
There was a problem hiding this comment.
BottommostLevelCompaction uses CamelCase. The same with lower case needs some separator, e.g. - (kebab-case) or _ (snake_case) for readability. In config property names - is commonly used.
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
|
@ptlrs Could you please drive this PR to completion by addressing the minor comments given? |
…ocksDB-BottommostLevelCompaction-options-configurable
…BottommostLevelCompaction configuration values.
There was a problem hiding this comment.
Thanks for updating the patch @ptlrs.
-
Please look into the CI build failure.
-
Let's update compaction CLI to also use enum names instead of int - #10428
It can also be done in a follow-up PR.
…on with enum and update configuration handling.
ptlrs
left a comment
There was a problem hiding this comment.
I have updated the CLIs to use the enum directly instead of ints or strings.
${COMPLETION-CANDIDATES} will render the valid values automatically.
bash-5.1$ ozone repair om compact -h
Usage: ozone repair om compact [-hV] [--dry-run] [--force] [--verbose]
[--blc=<bottommostLevelCompaction>]
--cf=<columnFamilyName> [--node-id=<nodeId>]
[--service-id=<omServiceId>]
CLI to compact a column family in the om.db. The compaction happens
asynchronously. Requires admin privileges. OM should be running for this tool.
--blc, --bottommost-level-compaction=<bottommostLevelCompaction>
BottommostLevelCompaction option for RocksDB
compaction. Valid values: kSkip,
kIfHaveCompactionFilter, kForce, kForceOptimized
Default: kSkip
--cf, --column-family=<columnFamilyName>bash-5.1$ ozone repair om compact --cf keyTable --blc test1
Invalid value for option '--bottommost-level-compaction': expected one of [kSkip, kIfHaveCompactionFilter, kForce, kForceOptimized] (case-sensitive) but was 'test1'
Usage: ozone repair om compact [-hV] [--dry-run] [--force] [--verbose]Also, the maven verify command is incorrectly flagging an unused dependency. It only sees the transitive dependency's usage as per the bytecode and ignores the parent dependency's direct usage.
| <!-- | ||
| OMConfigKeys references ManagedCompactRangeOptions.BottommostLevelCompaction, an enum | ||
| inherited from org.rocksdb.CompactRangeOptions. Bytecode resolves it to the rocksdb | ||
| class, so the analyzer does not see the hdds-managed-rocksdb reference. | ||
| --> | ||
| <ignoredUnusedDeclaredDependency>org.apache.ozone:hdds-managed-rocksdb:jar</ignoredUnusedDeclaredDependency> |
There was a problem hiding this comment.
Thanks @ptlrs for updating the patch.
Also, the maven verify command is incorrectly flagging an unused dependency. It only sees the transitive dependency's usage as per the bytecode and ignores the parent dependency's direct usage.
A better way to fix it is:
- allow usage of
CompactRangeOptions.BottommostLevelCompaction, since it's just anenum, safe to use directly:
Lines 1967 to 1969 in 1a94394
- import and use
CompactRangeOptions.BottommostLevelCompaction(inOMConfigKeysand elsewhere) instead ofManagedCompactRangeOptions.BottommostLevelCompaction - remove unused dependency
hdds-managed-rocksdbfromhadoop-ozone/common/pom.xml
There was a problem hiding this comment.
Thanks for the suggestion @adoroszlai. I have updated the PR.
…ns and clean up unused dependencies.
| <allowedImports> | ||
| <!-- Allow non-RocksObject classes. --> | ||
| <allowedImport>org.rocksdb.ColumnFamilyDescriptor</allowedImport> | ||
| <allowedImport>org.rocksdb.CompactRangeOptions</allowedImport> |
There was a problem hiding this comment.
Sorry for nitpicking: CompactRangeOptions extends RocksObject, so it should not be allowed, only CompactRangeOptions.BottommostLevelCompaction.
What changes were proposed in this pull request?
RocksDB allows multiple configurations for configuration the compaction strategy for the bottom-most layer. These configurations impact the compaction times.
This PR makes the compaction strategy configurable.
kSkipwhich is faster thankForceWhat is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15342
How was this patch tested?
CI: https://github.com/ptlrs/ozone/actions/runs/26857346964